fix[venom]: check operand position in mem2var promotion#5085
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: befec1ca7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not all2(self._is_pointer_use(inst, alloca_inst.output) for inst in uses): | ||
| return |
There was a problem hiding this comment.
Preserve escaping uses before promoting other allocas
When the destination alloca is visited before the source alloca, this per-alloca position check is order-dependent: for %b = alloca 32; %a = alloca 32; mstore %b, %a, %b passes this check as the address operand and line 86 rewrites the mstore away before %a is examined. %a then no longer has its value-operand escape in the DFG and can still be promoted/deleted, so the issue remains for allocas declared after their destination slot. The escape set needs to be determined from the original uses before mutating stores, or stores of any alloca pointer should block promotion that would remove them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified empirically — the order-swapped shape is handled correctly. With %b visited first:
%b = alloca 32
%a = alloca 32
mstore %b, %a
%x = mload %a
%y = mload %b
sink %x, %y
promoting %b rewrites the mstore to %alloca_b_0 = %a via InstUpdater, which maintains the DFG — so when %a is examined, its uses are [assign, mload], and the assign opcode fails _is_pointer_use, blocking promotion. Output after MakeSSA+Mem2Var:
%b = alloca 32
%a = alloca 32
%alloca_b_0 = %a
%x = mload %a
%y = %alloca_b_0
sink %x, %y
%a survives as a real allocation with a real load, and the pointer value flows into %b's replacement variable — which is exactly the promotion semantics for %b (its slot held the pointer value). Note the escape check exists to prevent the pointer bytes from being silently dropped, not to prevent the destination slot from being promoted; once the destination is a stack variable, the stored pointer value is preserved in SSA.
Added test_mem2var_stored_pointer_dest_promoted_first (722a274) pinning this shape.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5085 +/- ##
=======================================
Coverage 92.93% 92.94%
=======================================
Files 188 188
Lines 27840 27848 +8
Branches 4831 4833 +2
=======================================
+ Hits 25874 25882 +8
Misses 1315 1315
Partials 651 651 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
charles-cooper
left a comment
There was a problem hiding this comment.
lgtm, paging @harkal and @HodanPlodky for review
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
What I did
Fixes #5070.
Mem2Vardecided whether an alloca is promotable by looking only at the opcodes of its uses, not the operand position. An alloca used as the value stored by anmstore(i.e. a pointer escaping into memory,mstore %b, %a) was mistaken for a memory-slot access and promoted, silently deleting the store of the pointer value.How I did it
Added an
_is_pointer_usecheck requiring every use to place the alloca in the memory-address operand position:mload(single operand, always the address) is fine; formstore(operands[value, ptr]) andreturn(operands[size, ptr]), the alloca must not appear inoperands[0]. This also blocks the self-referentialmstore %a, %ashape. Operand-order semantics confirmed againstmemory_location.pyandload_elimination.py(venom operands are reversed relative to assembly).How to verify it
New
tests/unit/compiler/venom/test_mem2var.py: the issue reproducer assertsmstore %b, %asurvives MakeSSA+Mem2Var (fails on master, where the store is deleted), plus a positive case asserting ordinary promotion still happens. Fulltests/unit/compiler/passes.Commit message
Description for the changelog
Fix: Venom
Mem2Varno longer promotes allocas whose pointer value escapes as the stored value of anmstore.Cute Animal Picture